Skip to content

fix: correct underflow in random_updated_at#4363

Open
metalurgical wants to merge 11 commits intocowprotocol:mainfrom
metalurgical:fix/random-updated-at-underflow
Open

fix: correct underflow in random_updated_at#4363
metalurgical wants to merge 11 commits intocowprotocol:mainfrom
metalurgical:fix/random-updated-at-underflow

Conversation

@metalurgical
Copy link
Copy Markdown

@metalurgical metalurgical commented Apr 26, 2026

Description

Fix underflow when subtracting large Durations from Instant in random_updated_at.

Changes

  • Compute age as a percentage of max_age
  • Use saturating_mul to avoid overflow when calculating age
  • Clamp Instant subtraction underflow to now via checked_sub(...).unwrap_or(now)
  • Adds coverage tests

How to test

cargo nextest run -p price-estimation

@metalurgical metalurgical requested a review from a team as a code owner April 26, 2026 11:53
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the random_updated_at function to use floating-point multiplication for age calculation and introduces checked_sub to prevent underflow panics when the calculated age exceeds the current time. A unit test was also added to verify this behavior. Feedback was provided regarding the fallback logic, which still contains an unchecked subtraction that could potentially panic if the system uptime is less than one second.

Comment thread crates/price-estimation/src/native_price_cache.rs Outdated
@metalurgical metalurgical force-pushed the fix/random-updated-at-underflow branch from 4c187fa to b66e558 Compare April 26, 2026 14:53
Copy link
Copy Markdown
Contributor

@jmg-duarte jmg-duarte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after the saturaring_* changes, please test the edges of the rng (50% and 90%) explicitly too

Comment thread crates/price-estimation/src/native_price_cache.rs Outdated
Comment thread crates/price-estimation/src/native_price_cache.rs Outdated
Comment thread crates/price-estimation/src/native_price_cache.rs Outdated
@metalurgical
Copy link
Copy Markdown
Author

after the saturaring_* changes, please test the edges of the rng (50% and 90%) explicitly too

Good points, thanks.

Fix underflow when subtracting large durations from Instant in
random_updated_at.

- Compute age as a percentage of max_age
- Use saturating_mul to avoid overflow when calculating age
- Clamp Instant subtraction underflow to now via checked_sub(...).unwrap_or(now)
- Ensure percentage does not exceed 100
- Adds coverage tests
@metalurgical metalurgical force-pushed the fix/random-updated-at-underflow branch from b66e558 to c47d8dc Compare April 27, 2026 17:58
Copy link
Copy Markdown
Contributor

@jmg-duarte jmg-duarte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not force-push over the previous commits; at the end, everything will be squashed. Force pushing just makes my work harder because I can't diff between reviews

Comment thread crates/price-estimation/src/native_price_cache.rs Outdated
Comment thread crates/price-estimation/src/native_price_cache.rs
- Switch to `StdRng::seed_from_u64` for deterministic tests
- Remove `assert` on percentage <= 100
- Remove `rand_chacha` dependency.
@metalurgical metalurgical requested a review from jmg-duarte April 27, 2026 19:30
Copy link
Copy Markdown
Contributor

@jmg-duarte jmg-duarte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last note, everything else looks ok

Comment thread crates/price-estimation/src/native_price_cache.rs
Comment thread crates/price-estimation/src/native_price_cache.rs
@metalurgical metalurgical requested a review from jmg-duarte April 28, 2026 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants